Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: the cosine similarity is evaluated for top comments and bot comments are ignored #225

Open
wants to merge 17 commits into
base: development
Choose a base branch
from

Conversation

gentlementlegen
Copy link
Member

@gentlementlegen gentlementlegen commented Dec 26, 2024

Resolves #174

What are the changes

  • bot comments are ignored when building the prompt, which greatly reduces token usage and innacuracy
  • if the token count for the built prompt is higher than the model limit, strip the comments down to the 10 most relevant ones based on their cosine similarity with the specification

Rfc @sshivaditya2019

@gentlementlegen gentlementlegen marked this pull request as ready for review December 26, 2024 07:39
Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very skeptical of tfidf approach. We should go simpler and filter out more instead.

@@ -15,6 +15,9 @@ import {
import { BaseModule } from "../types/module";
import { ContextPlugin } from "../types/plugin-input";
import { GithubCommentScore, Result } from "../types/results";
import { TfIdf } from "../helpers/tf-idf";

const TOKEN_MODEL_LIMIT = 124000;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This depends on the model and possibly should be an environment variable because we might change models.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard coding the 12400 doesn't seem like a solution there either

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0x4007 It is not hard coded but configurable within the config file?
There is no API to retrieve a model max token value as far as I know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 179 is hard coded

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should I do if the configuration is undefined, should I throw an error and stop the run then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes if we don't have it saved in our library or collection of known amounts then it should throw

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no known amounts because no API nor endpoints can give this information, so I'll just throw when undefined then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manually get the numbers from their docs then

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that this number is arbitrary, diddn't you just ask OpenAI to raise the limits on the account we're using, with the same model as before and they did increase the limit? I'm afraid this number can't be guessed or hard-coded.

src/parser/content-evaluator-module.ts Outdated Show resolved Hide resolved
src/parser/content-evaluator-module.ts Outdated Show resolved Hide resolved
@shiv810
Copy link

shiv810 commented Dec 29, 2024

I don't think TF-IDF would be the best option for selecting the comments, as it only takes in account the word frequency and does not give any importance to semantics. A better solution might be to switch to a larger context model, like Gemini, which provides a 2 million token context window when we reach the token limit, and exclude bot-generated comments from the selection process.

@gentlementlegen
Copy link
Member Author

This was added as a failsafe when we go through the limit. Gemini can be an option, but theoretically we could also go beyond its token limit (even if unlikely). Since we can configure models which all have different token max limits (and third parties could be using smaller and cheaper ones) I think it is important that the technique we use to lower the context size is not based on LLM.

@0x4007
Copy link
Member

0x4007 commented Dec 29, 2024

More careful filtering of comments like removal of bot commands and comments, and possibly text summarization, or concatenation of multiple calls are all more accurate approaches. TFIDF is not the right tool for the job.

@gentlementlegen
Copy link
Member Author

The commands and bot comments got also fixed in this PR. I added this as a last resort if this was not enough. As I said before, I don't think it should rely on LLMs itself because third party users could chose a tiny model like gpt-3 and have only 3000 tokens available which would not be capable to even summarize. I can change this to summarize each comment in one sentence, which would use one call to the LLM per comment, which I though would potentially be expensive.

@0x4007
Copy link
Member

0x4007 commented Dec 29, 2024

Doing multiple calls to score everything and then concatenate results seems the most straightforward with no data loss.

@gentlementlegen
Copy link
Member Author

@0x4007 Doing the evaluation is not the problem, the problem is the given context to the LLM that gets too big. If an issue has 300 comments, then the prompt would contain these 300 comments while evaluating which would be too many for the token limit, so it has to get smaller. I don't see a way to fix that with no data loss, except if you meant comparing the comment against every other single comment one by one?

@0x4007
Copy link
Member

0x4007 commented Dec 30, 2024

Divide into two and do 150 each call. Receive the results array and concatenate them together

@gentlementlegen
Copy link
Member Author

gentlementlegen commented Dec 30, 2024

@0x4007 This is not reliable, and if a third party decides to use a model like o4-mini it would break anyways. What should be done in that case?

Plus concatenating would not make sense. I should run the comment against 150 first and 150 last (which would make the context inaccurate) and then probably average the result. I believe you didn't see how comment are evaluated: the problem is that for the context of understanding comments, we send all the comments in the context evaluated against the comments of the user. Refer to:

@0x4007
Copy link
Member

0x4007 commented Dec 31, 2024

Surely it's a bit of a trade off without all of the comments in one shot, but this approach seems to trades off the least.

  1. The task specification is by far the most important reference point.
  2. We can inject a new line in the prompt explaining that following is part 2/2 and that due to context window limits we had to split the conversation into multiple parts for evaluation.

and if a third party decides to use a model like o4-mini it would break anyways. What should be done in that case?

Why would they complain about using a non default model? We set the default to what we know works.

@gentlementlegen gentlementlegen marked this pull request as draft January 3, 2025 08:31
@gentlementlegen gentlementlegen marked this pull request as ready for review January 4, 2025 03:49
@gentlementlegen
Copy link
Member Author

@0x4007 Changed the behavior when the limit of the model is burst:

  • split the prompt by n chunks
  • average the list of scores by comment

.cspell.json Outdated Show resolved Hide resolved
# Conflicts:
#	dist/index.js
@gentlementlegen
Copy link
Member Author

gentlementlegen commented Jan 8, 2025

 [ 115.94 WXDAI ] 

@gentlementlegen
Contributions Overview
ViewContributionCountReward
IssueTask1100
IssueSpecification115.94
Conversation Incentives
CommentFormattingRelevancePriorityReward
> ```diff@gentlementlegen perhaps we have too m…
7.97
content:
  content:
    p:
      score: 0
      elementCount: 2
    em:
      score: 0
      elementCount: 1
    a:
      score: 5
      elementCount: 1
  result: 5
regex:
  wordCount: 54
  wordValue: 0.1
  result: 2.97
1215.94

 [ 21.956 WXDAI ] 

@0x4007
Contributions Overview
ViewContributionCountReward
IssueComment10.69
ReviewComment1921.266
Conversation Incentives
CommentFormattingRelevancePriorityReward
I think high accuracy is the best choice from your selection. I …
1.38
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 22
  wordValue: 0.1
  result: 1.38
0.2520.69
Very skeptical of tfidf approach. We should go simpler and filte…
0.94
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 14
  wordValue: 0.1
  result: 0.94
0.1520.282
This depends on the model and possibly should be an environment …
1.11
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 17
  wordValue: 0.1
  result: 1.11
0.420.888
We should also filter out slash commands? And minimized comments?
0.71
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 10
  wordValue: 0.1
  result: 0.71
0.320.426
I'm skeptical about this whole TFIDF approach1. The tokenizer a…
8.64
content:
  content:
    p:
      score: 0
      elementCount: 1
    ol:
      score: 1
      elementCount: 1
    li:
      score: 0.5
      elementCount: 3
  result: 2.5
regex:
  wordCount: 127
  wordValue: 0.1
  result: 6.14
0.3526.048
Can you articulate the weaknesses or concerns
0.52
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 7
  wordValue: 0.1
  result: 0.52
0.2520.26
Hard coding the 12400 doesn't seem like a solution there either
0.83
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 12
  wordValue: 0.1
  result: 0.83
0.220.332
Line 179 is hard coded
0.39
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 5
  wordValue: 0.1
  result: 0.39
0.1520.117
Yes if we don't have it saved in our library or collection of kn…
1.28
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 20
  wordValue: 0.1
  result: 1.28
0.2520.64
It shouldn't affect it at all. I would proceed with implicit app…
1.44
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 23
  wordValue: 0.1
  result: 1.44
0.120.288
Manually get the numbers from their docs then
0.59
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 8
  wordValue: 0.1
  result: 0.59
0.320.354
Why is this a constant? Makes more sense to use let and directly…
1.28
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 20
  wordValue: 0.1
  result: 1.28
0.421.024
```suggestion```
0
content:
  content: {}
  result: 0
regex:
  wordCount: 0
  wordValue: 0.1
  result: 0
0.0520
Add more chunks if the request to OpenAI fails for being too lon…
2.1
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 36
  wordValue: 0.1
  result: 2.1
0.4521.89
@shiv810 rfc
0.18
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 2
  wordValue: 0.1
  result: 0.18
0.120.036
Separate is fine then just as long as the current code is stable.
0.88
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 13
  wordValue: 0.1
  result: 0.88
0.2520.44
More careful filtering of comments like removal of bot commands …
2.05
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 35
  wordValue: 0.1
  result: 2.05
0.4521.845
Doing multiple calls to score everything and then concatenate re…
1.17
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 18
  wordValue: 0.1
  result: 1.17
0.42520.995
Divide into two and do 150 each call. Receive the results array …
1.06
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 16
  wordValue: 0.1
  result: 1.06
0.37520.795
Surely it's a bit of a trade off without all of the comments in …
6.58
content:
  content:
    p:
      score: 0
      elementCount: 1
    ol:
      score: 1
      elementCount: 1
    li:
      score: 0.5
      elementCount: 2
  result: 2
regex:
  wordCount: 90
  wordValue: 0.1
  result: 4.58
0.3524.606

 [ 2.456 WXDAI ] 

@shiv810
Contributions Overview
ViewContributionCountReward
ReviewComment32.456
Conversation Incentives
CommentFormattingRelevancePriorityReward
@gentlementlegen It shouldn't impact the comment evaluation at a…
1.7
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 28
  wordValue: 0.1
  result: 1.7
0.620.516
Besides the error code, `OpenRouter` provides `Provi…
1.38
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 22
  wordValue: 0.1
  result: 1.38
0.820.56
I don't think TF-IDF would be the best option for selecting the …
3.66
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 69
  wordValue: 0.1
  result: 3.66
0.7521.38

This is the result I got while limiting token count to 2000 which led to comment splitting. Note that I use the default configuration not ubiquity-os-marketplace. Target was ubiquity-os-marketplace/text-conversation-rewards/174

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check overhead for comment evaluation
3 participants